-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
write_tsdb plugin: Export metadata. #1655
base: main
Are you sure you want to change the base?
Conversation
src/daemon/meta_data.c
Outdated
} | ||
|
||
pthread_mutex_lock (&orig->lock); | ||
for(e=orig->head; NULL != e; e = e->next) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed this one.
@kev009 I did review #1106 yesterday, see #1106 (comment) |
Ok, so this one can be closed? |
Ok, this is mergable, if anyone desires any change/squash of git history I am happy to accommodate. I'm going to try and look harder at all this based on @mfournier's comment in #1656, but this PR shouldn't need any changes as long as the meta_data API stays the same. |
Any news on this being merged, we're really keen to use it without building our own packages |
Looking forward to this feature being merged. |
@rubenk I'm thinking this should be merged as it stands, and we can look into perf of metadata later on |
I'm not too worried about the performance aspect. What does worry me is the raciness and memory leak as mentioned in #1656 (comment) I'd rather address those before this goes into the tree than after, since if there are design issues, they are going to be much harder to fix when we have existing users of this feature. |
Sorry, but this is a bit too stale to rush this into 5.6. Our current plans are to release 5.7 in December, this PR should make it into that I hope. |
we have been using this in production w/o problem - definitely critical to support query drill-downs for values with large entry count when using Grafana |
FYI, this features like this one have been requested for a number of write plugins. Right now, the meta data is an internal concept only. We're not opposed to exporting this, but we'd like to do it right. There should be a design and discussion before we start accepting such changes into the codebase. I'll likely create a feature request issue to track this and write up a design proposal. Stay tuned. Best regards, |
Hey guys, Any news on this PR ? Is there any decision taken ? Thanks a lot, |
Enabled |
ad01371
to
8c51cbc
Compare
I've rebased the code on master and reformatted with clang-format. |
@octo what is the architecture path forward here? It seems like many types of metrics stores benefit greatly from this principle of plugins to push metadata, but there needs to be clear direction for feeding it from collectd and plugins. |
Is there a plan to go forward with this feature? our team can really use this feature. |
@bzed no progress, I need someone to help brainstorm the design due to time constraints. Happy to update this once we have an API design figured out. |
Merge conflict resolution and fixes for @rubenk review